Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the AddHoldInvoice functionality by introducing more flexible preimage and hash management. It allows users to either provide a preimage for the server to derive a hash from, or opt for the server to auto-generate both a preimage and hash. The changes include updates to the RPC interface, command-line tool, and server-side logic, ensuring backward compatibility while providing robust error handling for invalid configurations.

Highlights

• AddHoldInvoice Enhancements: Updated AddHoldInvoice to support optional preimage/hash generation, allowing the server to auto-generate a preimage or derive a hash from a user-supplied preimage.
• RPC Interface Updates: Modified the AddHoldInvoiceRequest and AddHoldInvoiceResp protobuf definitions to include new fields for preimage and hash handling.
• New Integration Tests: Added a new integration test suite, testHoldInvoiceAutoGenerate, to verify the new functionality across various modes including auto-generation, user-supplied preimage, and legacy hash-only flows.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Design question: should --preimage exist as input, and should the preimage be stored in the DB?

Thanks for the PR — the auto-generate feature is a real quality-of-life improvement and the core implementation is solid. I want to raise two related design questions before this lands.

1. Should bytes preimage exist as an input field?

The stated purpose is to let callers avoid computing SHA256(preimage) themselves. But that's a trivial operation — every Lightning library exposes it, and lnd itself has preimage.Hash() in lntypes. The savings are minimal.

In exchange, the feature:

• Sends the preimage over the RPC channel (potentially captured in logs, middleware, audit layers)
• Stores the preimage in the DB at invoice creation time — before any payment has arrived, before the merchant has decided to settle

The original hold invoice design was deliberate: the merchant keeps the preimage locally, passes only the hash to AddHoldInvoice, and the preimage never touches lnd's DB until SettleInvoice is called. That's the whole point — the merchant controls exactly when the preimage is revealed.

The correct flow for a caller who already has a preimage is just:

hash = SHA256(preimage)
AddHoldInvoice(hash=hash)

My suggestion: remove bytes preimage from AddHoldInvoiceRequest and the corresponding CLI flag. The auto-generate path (neither hash nor preimage provided) is where the real value is.

2. Should the preimage be stored in the DB even for the auto-generate case?

Currently even the auto-generate path stores the preimage in ContractTerm.PaymentPreimage via AddInvoiceData.Preimage. But SettleInvoice always requires the caller to supply the preimage — lnd never reads it from the DB to perform settlement. So storing it serves no functional purpose.

The cleaner approach: generate the preimage locally in AddHoldInvoice, pass only the hash to AddInvoice (Preimage: nil), and return the preimage directly from the local variable in the response — never writing it to the DB:

default: // auto-generate
    var preimage lntypes.Preimage
    if _, err := rand.Read(preimage[:]); err != nil {
        return nil, fmt.Errorf("failed to generate preimage: %w", err)
    }
    h := preimage.Hash()
    hashPtr = &h
    autoPreimage = &preimage  // returned in response, not stored

This preserves the hold invoice invariant: the DB only ever holds the hash, never the preimage. The preimage is returned once in the response (the caller must store it, like a seed phrase), and provided back at settle time. It also eliminates a whole class of future bugs where a preimage stored in the DB could be used inadvertently if the HodlInvoice flag check ever has a regression.

To be clear — none of this blocks the auto-generate feature itself, which is good and should land. The question is just whether we want to keep --preimage as input and whether the preimage should live in the DB.

🟠 PR Severity: HIGH | 5 files | 217 lines changed | HIGH: lnrpc/invoicesrpc/invoices_server.go (lnrpc/* RPC server) | MEDIUM: cmd/commands/invoicesrpc_active.go (cmd/* CLI), lnrpc/invoicesrpc/invoices.proto (.proto), lnrpc/invoicesrpc/invoices.swagger.json | LOW: docs/release-notes/release-notes-0.22.0.md | EXCLUDED (test/generated): itest/list_on_test.go, itest/lnd_hold_invoice_auto_generate_test.go, lnrpc/invoicesrpc/invoices.pb.go | No bump: 5 files under threshold 20, 217 lines under threshold 500. To override add severity-override-{critical,high,medium,low} label. <!-- pr-severity-bot -->

Hi @ziggie1984 @yyforyongyu the feedback has been addressed. The pr is ready for your review. Thanks

@yyforyongyu: review reminder
@ziggie1984: review reminder
@saubyk, remember to re-request review from reviewers when ready

My suggestion: remove bytes preimage from AddHoldInvoiceRequest and the corresponding CLI flag.

I don't understand this. I don't see bytes preimage in AddHoldInvoiceRequest.

2. Should the preimage be stored in the DB even for the auto-generate case?

I think it should. We already do this for normal (non-hold) invoices.

Also, requiring the client to persist the preimage requires that client to persist more state that they may not want to do (which we don't require for normal (non-hold) invoices). I think the state should be held within LND if the preimage is generated by LND.

If there is some use case where we don't want LND to know the preimage before we are ready to settle, there is no way to prove that LND forgot the preimage if LND generate the preimage, so I see no advantage in LND not saving the preimage if it generated it.

Additionally, if LND saves the preimage, we can have another option to auto-settle after X number of blocks, rather than only be able to auto-cancel. Or, allow another client that did not generate the preimage (or receive the preimage) to settle the invoice.

This could help with users of #3120 who want to deal with hold invoices that may have one client dealing with all hold invoices on a node.

Generally, my understanding of the current design was that when we force the client to generate the preimage themselves, we are sort of forcing them to realize that they are creating something unique and they need to persist it in order to be able to settle the hold invoice. If we instead generate the preimage in LND and return it to the client, but LND forgets the preimage, the client may not realize they needed to persist the preimage that was returned to them.

I definitely think we should still give the user the option to generate and persist their own preimage if they have unique application that requires it to be done externally.

If we are going to give the functionality where they don't have to generate the preimage on their own (which is a nice alternative to give users), we should also not require them to persist it.

I am generally not sure about this feature as a whole now. Hold-Invoices and normal invoices are by design separated. Hold-Invoices need to be settled or canceled via an external API (will be canceled after a timeout). So when setteling the invoice the caller needs to provide the preimage anyways so I would still push back persisting the preimage here. Providing the autogeneration of the preimage and hash can make sense to define the standards for the caller but persisting the preimage I would not do for now.